Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libmetal: add metal_list_for_each_safe() support #251

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

yintao707
Copy link

No description provided.

@yintao707 yintao707 force-pushed the listsafe branch 4 times, most recently from 0f6197b to 09cb9bc Compare September 12, 2023 06:31
@yintao707
Copy link
Author

yintao707 commented Sep 12, 2023

hi @arnopo ,
Please help review this modification,this adds metal_list_for_each_safe for libmetal.
thanks

Copy link
Contributor

@arnopo arnopo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I suppose that you plan then to update the openamp lib using it?

@arnopo
Copy link
Contributor

arnopo commented Sep 12, 2023

hi @yintao707,
Could you rebase the PR on main branch, I just fix the the "check buids" CI test

@yintao707
Copy link
Author

hi @yintao707, Could you rebase the PR on main branch, I just fix the the "check buids" CI test
OK,I will rebase later, thanks

@yintao707
Copy link
Author

hi, @arnopo, @edmooring , @tnmysh ,
May I ask if this modification can be merged ?
thanks

@arnopo
Copy link
Contributor

arnopo commented Sep 18, 2023

hi, @arnopo, @edmooring , @tnmysh , May I ask if this modification can be merged ? thanks

Please let's time to people to review the patches even if quite simple. One implicite rule is that 2 experts or maintainers has approved the PR.

lib/list.h Outdated
@@ -98,6 +98,10 @@ static inline struct metal_list *metal_list_first(struct metal_list *list)
(node) != (list); \
(node) = (node)->next)

#define metal_list_for_each_safe(list, node, temp) \
Copy link
Collaborator

@tnmysh tnmysh Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While other APIs are not documented, I prefer we document this API in doxygen format. This will clearly say what is head, node etc..
Ideally I would prefer if we keep order of arguments matching with kernel API of list_for_each_safe. It makes developer's life easy.
@arnopo do you have any preference?

https://github.com/torvalds/linux/blob/2cf0f715623872823a72e451243bbf555d10d032/include/linux/list.h#L723

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with both of Tanmay's points.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes make sense
Please @yintao707, update based on @tnmysh remarks

Copy link
Contributor

@CV-Bowen CV-Bowen Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tnmysh @arnopo OK, we will update this PR, include two commits:

  1. document the list API in doxygen format.
  2. add metal_list_for_each_safe() support.

Change the arguments order will affect a lot of code, so let's change this in the other PR,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes make sense Please @yintao707, update based on @tnmysh remarks

hi, @arnopo , @tnmysh, @edmooring ,
I have updated this PR, I am not sure if the comments I added are compliant. Please help me review it again, thanks.​
The parameters of this API need to be in the same order as the parameters of the Linux API, I think that metal_list_for_each should be modified firstly, this can ensure that the two APIs are consistent, howerver, which will have a lot of impact. so, let's change this in the other PR.

lib/list.h Outdated Show resolved Hide resolved
Add a more secure way to traverse linked lists

Signed-off-by: Guiding Li <liguiding1@xiaomi.com>
Copy link
Collaborator

@tnmysh tnmysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@arnopo arnopo requested a review from CV-Bowen October 18, 2023 15:01
@arnopo arnopo added this to the Release V2024.04 milestone Oct 18, 2023
Copy link
Contributor

@CV-Bowen CV-Bowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@arnopo
Copy link
Contributor

arnopo commented Oct 19, 2023

This PR is ready to be merged. It will be merged just after the Release v2023.10
thanks,

@arnopo arnopo merged commit e087ea5 into OpenAMP:main Oct 31, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants